-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: improve branch support logic #301
Conversation
src/index.ts
Outdated
); | ||
|
||
if (!branch.trim()) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that the trim()
call is redundant after map ((b) => b.trim())
, but do we need to still check whether branch
is empty? Or add a .filter()
after .map()
to remove empty strings?
9fe38a7
to
c833283
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I am missing something here, but it looks like this change moves the check for branch existence to only fire when NO_EOL_SUPPORT is set?
try { | ||
await context.octokit.repos.getBranch(context.repo({ branch })); | ||
} catch (err) { | ||
robot.log( | ||
`${branch} does not exist - no backport will be initiated`, | ||
); | ||
await context.octokit.issues.createComment( | ||
context.repo({ | ||
body: `Provided branch \`${branch}\` does not appear to exist.`, | ||
issue_number: issue.number, | ||
}), | ||
); | ||
continue; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this block moved under the section flagged by NO_EOL_SUPPORT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSupportedBranches
itself checks branch existence - so in moving it down we save a redundant check in the case the variable is set.
it moves the conditional down, but not inside the check - if |
c833283
to
54c81af
Compare
Secondary to #300
Cleans up some comment-initiated backport logic